Skip to content

Work On Issue 18693 - Remove rndtonl from std.math #6429

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
dlang-bot merged 1 commit into from
Apr 10, 2018
Merged

Work On Issue 18693 - Remove rndtonl from std.math #6429

dlang-bot merged 1 commit into from
Apr 10, 2018

Conversation

@JackStouffer
Copy link
Contributor

@JackStouffer JackStouffer commented Apr 6, 2018

This function

  1. Results in link errors if you're not using DM's C runtime
  2. Is non-standard
  3. Is covered by round and friends

Side note: all of these platform specific math functions which share the C names feel a lot like PHP. This is not how these would be designed if std.math was to be written today, so any instance of these that we can remove/redesign, the better.

@dlang-bot
Copy link
Contributor

dlang-bot commented Apr 6, 2018

Thanks for your pull request, @JackStouffer!

Bugzilla references

Auto-close Bugzilla Severity Description
✗ 18693 normal std.math.rndtonl and core.math.rndtonl result in link errors

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#6429"

@JackStouffer
Copy link
Contributor Author

JackStouffer commented Apr 6, 2018

Looks like ddox is mis-parsing the unittest block and including the next function signature

@wilzbach
Copy link
Contributor

wilzbach commented Apr 9, 2018

Both at the time of deprecation and removal, a changelog entry must be made. This
changelog entry should have a short motivation for the deprecation (or removal)
and should describe which steps can be taken by the user to upgrade their codebase.

(from dlang/DIPs#108)

@wilzbach wilzbach added the Review:Needs Changelog A changelog entry needs to be added to /changelog label Apr 9, 2018
wilzbach
wilzbach approved these changes Apr 9, 2018
View reviewed changes
Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note: all of these platform specific math functions which share the C names feel a lot like PHP. This is not how these would be designed if std.math was to be written today, so any instance of these that we can remove/redesign, the better.

Fully agreed. std.math would look quite differently today.

@wilzbach
Copy link
Contributor

wilzbach commented Apr 9, 2018 •
edited
Loading

@andralex could we get your approval on this one? Thanks!

@JackStouffer JackStouffer removed the Review:Needs Changelog A changelog entry needs to be added to /changelog label Apr 9, 2018
@JackStouffer JackStouffer force-pushed the issue18693 branch 2 times, most recently from f32aefe to 0c2173b Compare April 10, 2018 13:58
andralex
andralex approved these changes Apr 10, 2018
View reviewed changes
Copy link
Member

@andralex andralex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If semantics are not identical, please specify differences in the deprecation message and changelog. Thanks!

@JackStouffer
Copy link
Contributor Author

JackStouffer commented Apr 10, 2018

assert(rndtonl(1.0) is -real.nan);

does this function even work at all

@JackStouffer
Copy link
Contributor Author

JackStouffer commented Apr 10, 2018

does this function even work at all

Nope.

@dlang-bot dlang-bot merged commit 813327d into dlang:master Apr 10, 2018
@JackStouffer JackStouffer deleted the issue18693 branch April 10, 2018 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andralex andralex andralex approved these changes

@ibuclaw ibuclaw Awaiting requested review from ibuclaw ibuclaw is a code owner

@dnadlinger dnadlinger Awaiting requested review from dnadlinger

+1 more reviewer

@wilzbach wilzbach wilzbach approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

math Merge:auto-merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@JackStouffer @dlang-bot @wilzbach @andralex